Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RHOAIENG-15152: feat(codeserver/e2e): add initial playwright e2e test project skeleton #755

Merged

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented Oct 27, 2024

Made for https://issues.redhat.com/browse/RHOAIENG-14518 but it's essentially unrelated work.

Filled new Jira just for the test https://issues.redhat.com/browse/RHOAIENG-15152

Description

As you are well aware, we've been struggling to pick our e2e toolset and to write some left-shifted tests. Thankfully, we have a helpful Architect in Red Hat Middleware-Application Services (hello David), who recommended Playwright in TypeScript as the only sensible e2e option for web stuff that even developers are willing to touch.

I did consider Cypress and various Selenium bindings; I would not mind switching what I have here into that if it's more liked in the team. Just I think we should polish and merge this Playwright first, even if we're going to abandon it, so that we at least have something.

The await porn is even more stupid than I expected that to be. Mandatory reading https://journal.stuffwithstuff.com/2015/02/01/what-color-is-your-function/ And there's some theory about effect systems if you want to discuss this with academically minded people!?!

I'm using the Playwright test runner. That's by no means required; it's possible to use just the automation library part of Playwright and structure the tests differently. I guess I decided that since I don't have any test runner I like in the JavaScript world, I may just as well pick the Playwright one. It has fixtures like Pytest, so there may be some familiarity there.

Besides that, we have here testcontainers, because they are also the most popular and therefore should be relatively unobjectionable choice for a library to start a container in the background.

What does it look like

Playwright has a fancy html report. I do pretty much agree with @zhimin's prolific blogs that are saying that all that is not actually well suited for a traditional QE team (edit: that's working in true agile way), but we aren't that, and also the number of e2e tests we're going to have here to run on every PR is going to be small. We're still free to test differently in QE (or DevTestOps as we now say).

Screenshot 2024-10-28 at 12 33 16 PM

What's it like to work with

The tests can be set to run against fresh browser as well as a browser you provide (the --remote-debugging-port is what does the trick). Check out playwright.config.ts where you can switch between the two modes.

/Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome --remote-debugging-port=9222 --user-data-dir=./data23 --enable-automation --no-first-run

In the same vein, the tests can run against already started codeserver image, or they can start its own container, fresh one for every test. (I'm not sure if we should try to reuse codeserver instance or start fresh every time. Fresh should make writing tests easier as cleanup becomes unnecessary and the cost in increased test runtime is I think worth that. If we add more suites besides this "basic functionality" one, then maybe we can reassess that there. Good test hygiene is hard, especially keeping it up for a longer time.)

podman run -p 8787:8787 --rm -it quay.io/opendatahub/workbench-images:codeserver-ubi9-python-3.11-2024b_20241026

Debugging tests

Running against provided browser and provided codeserver, possibly commenting out few unnecessary steps before the interesting part, it is usually possible to iterate on the thing quickly and get it working. Then check with fresh browser and fresh image.

Playwright has the codegen and test --ui tools. The codegen is ok to give you decent locators to use, and the ui is not bad either; video recording on steroids.

The test --ui thing is pretty much what Cypress has also; the thing we were so impressed with in Savitha's demo earlier this year.

For some reason, default Playwright settings will popup a new browser tab with report every time a test run fails. I've disabled that popup thing and instead enabled the line reporter that gives you some stacktrace; thats way more reasonable.

I do have two fancy test helpers in some utils file. They are sufficiently complex that there may need to be special tests to test these tests (helpers) ;P

The tests themselves

I essentially copied that from Coder Inc. I did preserve copyright header and put MIT licence, same as they have, so we should be good. The difference from them is that they control code-server invocation, whereas for us the parameters given to it are fixed. So my tests can be simpler.

How Has This Been Tested?

GHA and played with it on my machine.

image

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

I'll cleanup the git history later. Please have a look now nevertheless.

@jiridanek jiridanek added the trivy-scan This label that allows trivy to create a security report on the pull requests label Oct 27, 2024
@jiridanek jiridanek self-assigned this Oct 27, 2024
@openshift-ci openshift-ci bot requested review from harshad16 and paulovmr October 27, 2024 23:12
@jiridanek
Copy link
Member Author

jiridanek commented Oct 28, 2024

The CI fails are missing --- in the pnpm lock file yaml (edit: fixed now), and unreliable trivy scanner (edit: still unreliable). For --- I think we should put that file into ignore, and for trivy not sure what to do. Ganesh (former colleague of sorts) just disabled it in their

@jiridanek
Copy link
Member Author

One funny realization; depending on the platform where you run codeserver (or most likely where and what browser you're opening it with), the buttons for trusting and not trusting workspace are switched. Either the ack button is on the left, or on the right. Never realized that it actually changed order for me after I switched from KDE Plasma to MacOS!

@@ -134,7 +134,7 @@ jobs:
if: "${{ fromJson(inputs.github).event_name == 'pull_request' }}"
env:
IMAGE_TAG: "${{ github.sha }}"
IMAGE_REGISTRY: "localhost:5000/workbench-images"
IMAGE_REGISTRY: "ghcr.io/${{ github.repository }}/workbench-images"
Copy link
Member Author

@jiridanek jiridanek Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We aren't pushing the image any more as we had to in the beginning, so we can use any registry we like; so let's keep it consistent.

@jiridanek jiridanek changed the title NO-ISSUE: feat(codeserver/e2e): add initial playwright e2e test project skeleton RHOAIENG-15152: feat(codeserver/e2e): add initial playwright e2e test project skeleton Oct 29, 2024
@jiridanek jiridanek force-pushed the jd_codeserver_playwright branch from dcbd1ea to 42f0d28 Compare November 12, 2024 17:24
@jiridanek
Copy link
Member Author

/cc @caponetto @jstourac @atheo89

@jiridanek jiridanek added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 12, 2024
@jstourac
Copy link
Member

I didn't actually run this on my machine. LGTM in general. Do we want to create this stub only in the codeserver indeed? If we're introducing this, don't we want to create it somehow generic, so we can reuse at least portions of code/tests between the diferent images?

Also, looks like the Notebooks 2.0 will use Cypress in the end: kubeflow/notebooks#75. Should we be concerned? 🤔

@jiridanek
Copy link
Member Author

jiridanek commented Nov 13, 2024

Here's nothing to run, actually. It's not a walking skeleton, just a skeleton.

I originally intended to keep this in code server for the time being, but you're right that maybe starting in tests/e2e or some directory like that makes more sense. I'll do that change tomorrow.

  • move tests under /tests/browser

Regarding cypress, I am actually not concerned. Playwright is a better design (real browser automation library) more suitable for what we're doing. if we ever wanted to copy code from somebody else's cypress tests, we can do that with not too much effort.

@jiridanek jiridanek force-pushed the jd_codeserver_playwright branch from 42f0d28 to 630eec6 Compare November 13, 2024 19:30
Copy link
Contributor

openshift-ci bot commented Nov 13, 2024

@jiridanek: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/codeserver-notebook-e2e-tests dcbd1ea link true /test codeserver-notebook-e2e-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@jiridanek
Copy link
Member Author

/override ci/prow/images
not touching no images

Copy link
Contributor

openshift-ci bot commented Nov 14, 2024

@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/images

In response to this:

/override ci/prow/images
not touching no images

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link
Member

@jstourac jstourac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@jiridanek
Copy link
Member Author

/approve
thanks

Copy link
Contributor

openshift-ci bot commented Nov 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jiridanek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 768ae5c into opendatahub-io:main Nov 14, 2024
7 checks passed
@jiridanek jiridanek deleted the jd_codeserver_playwright branch November 14, 2024 17:22
jiridanek added a commit to jiridanek/notebooks that referenced this pull request Dec 18, 2024
… project skeleton (opendatahub-io#755)

* NO-ISSUE: feat(codeserver/e2e): add initial playwright project skeleton

* NO-ISSUE: feat(codeserver/e2e): add a basic README.md

* NO-ISSUE: add testcontainers as a dependency

* NO-ISSUE: new customizable and flexible config

* check env variables to determine how to run
* take screenshots on failure
* don't open annoying html report
* disable parallel runs

* exclude pnpm-lock.yaml from yamllint

* move Playwright tests from codeserver/e2e to tests/browser

* create a new containers folder for (future) testing of containers in Python

(cherry picked from commit 768ae5c)
jiridanek added a commit to jiridanek/notebooks that referenced this pull request Dec 19, 2024
… project skeleton (opendatahub-io#755)

* NO-ISSUE: feat(codeserver/e2e): add initial playwright project skeleton

* NO-ISSUE: feat(codeserver/e2e): add a basic README.md

* NO-ISSUE: add testcontainers as a dependency

* NO-ISSUE: new customizable and flexible config

* check env variables to determine how to run
* take screenshots on failure
* don't open annoying html report
* disable parallel runs

* exclude pnpm-lock.yaml from yamllint

* move Playwright tests from codeserver/e2e to tests/browser

* create a new containers folder for (future) testing of containers in Python

(cherry picked from commit 768ae5c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. trivy-scan This label that allows trivy to create a security report on the pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants